-
Notifications
You must be signed in to change notification settings - Fork 332
gptel-context: add file path expansion to prevent duplicates #1113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This prevents issues where the same file might be added multiple times
due to different path representations (e.g., relative vs absolute
paths, or paths with . and .. components).
This resolves an issue where the following gptel-context-move-to-tail
function would add the same file multiple times when the
buffer-file-name and the file path in gptel-context differed due to
path normalization.
```
(defun gptel-context-move-to-tail ()
"Move the current buffer's file to the tail of the context."
(when (and gptel-context
(member (buffer-file-name) (apply #'append gptel-context)))
(gptel-context-remove (buffer-file-name))
(gptel-add-file (buffer-file-name))))
(add-hook 'after-save-hook #'gptel-context-move-to-tail)
(add-hook 'after-revert-hook #'gptel-context-move-to-tail)
```
c6de920 to
6afe260
Compare
|
@aagit Thanks. This has one knock-on effect that might be undesirable: the context string will now include the full path to the file, which you may not want to disclose. See #956 for prior discussion on this. To be clear, if you call |
|
I'm calling it from a my code, so I can restore the cached context even if I restart the editor. I didn't realize gptel-add-file always uses the full path so the likelyhood of involountary dups during interactive use is already greatly reduced. |
|
@aagit Do you still want to merge this PR? It's unclear if the issue is resolved. |
|
It's ok, it's no big deal to add the absolute file path is already used by gptel-add and cannot cause "interactive" dupicates. For the script, if there's a good reason to keep a way not to use the absolute file path I guess it can be left as is. However maybe it would have been better to add a new command to use only non interactively that allows to bypass the absolute file path, and to apply my patch, that would cause less risk of involuntary dups, or alternatively to add a gptel-variable that defines a "prefix" to cut from all context paths if present in the context filename. That I think is less error prone for involountary "dups". I did run into dups myself because I had the files added by the gptel-add command, and the script added them also but with the relative path. I'll show what I have here in my initi file to put things in better prospective: Then I keep a script that loads the same directories in the same order at the head of context but occasionally I may edit those files and they get added to the absolute file path. I also can't figure out why the (unless (local-variable-p 'gptel-local-compilation-added) doesn't work to move the compilation buffer always at the end of context, it successful at adding the compilation always at the last version but it doesn't move to the tail and I need it to move to the tail to reduce the kvcache computations. I shall also filter out by name compilation currently it also adds all grep-alike buffers and that's a mistake. The above works perfectly but it does more context-remove/add churn than needed during compilations, not a big deal though. Overall with these tweaks I can work at >100k context with a single GPU with relatively low delays. |
|
If you're adding files non-interactively, you can just manipulate (cl-pushnew (expand-file-name (buffer-file-name)) gptel-context :test 'equal)And you can insert it anywhere you want in the list. |
|
Ok I replaced it with this, should reduce some overhead. So now I noticed something strange. If I edit the context every time I press C-c C-c in context-mode the context reverses order. |
I thought the context buffer might be reversing |
|
You should be able to reproduce the reversal with C-c C-c if you first delete an entry with "d". After C-c C-c commits the deletion of an entry any further C-c C-c reverses the order. I tried to disable the above three hooks and it still happened. |
|
Another detail relevant to this pull request, gptel-add-file does not add the absolute path name without this commit if the path picked is ~/.. however buffer-file-name in my hooks above is always absolute, so out of the box the interactive usage of gptel-add-file vs hooks won't work. So overall I think it's preferable to always use expand-file-name in gptel-add-file: if someone wants to use relative paths it's still possible by manipulating gptel-context by hand, and then one should know better to avoid duplicates. |
This prevents issues where the same file might be added multiple times due to different path representations (e.g., relative vs absolute paths, or paths with . and .. components).
This resolves an issue where the following gptel-context-move-to-tail function in my init file would add the same file multiple times when the buffer-file-name and the file path in gptel-context differed due to path normalization.